Skip to content

feat: bb uses goblin#3636

Merged
ledwards2225 merged 54 commits into
masterfrom
cg-lde/expose-goblin
Dec 18, 2023
Merged

feat: bb uses goblin#3636
ledwards2225 merged 54 commits into
masterfrom
cg-lde/expose-goblin

Conversation

@codygunton

@codygunton codygunton commented Dec 8, 2023 •
edited by ludamad
Loading

Copy link
Copy Markdown
Contributor

"vertical slice" of goblin integrating with ACIR and BB.

Beginning of goblin ultra honk interface for bb. Can only run a basic test (assert_statement) for now and is not intended for use outside of the CI check that it's still working, for now.

  • adds bb and bb.js command prove_and_verify_goblin. Adds bb.js bindings for goblin prove/verify
  • modifies ACIR dsl folder to be able to take a goblin builder
  • adds CI calls to bb.js and bb that use prove_and_verify_goblin
  • allowing ability to load grumpkin SRS through memory, needed for bb and (especially) bb.js. This allows an alternate source of points to be used other than the default file-based grumpkin loader, which mostly only works in dev (though revisit: could this work for native bb?)

kevaundray
kevaundray reviewed Dec 9, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/dsl/acir_proofs/major-todos.md Outdated

Make acir_format and acir_proofs tests work

Stetch: try to understand what is_recursive flag should do

@kevaundray kevaundray Dec 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We allow two proving systems to be created, once of them is "snark friendly" meaning that it uses PedersenBlake in the transcript and the other is solidity friendly meaning that it uses keccak in the transcript.

When creating a proof that we will want to verify in a another circuit, we use the is_recursive flag.

See https://hackmd.io/@6iQDuIePQjyYBqDChYw_jg/SyLHcKsSa for a few more details

@codygunton codygunton Dec 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining. Our plans is to do violence to the code to get something working in the non-recursive case this week then check in with you and Maxim (prob next year?) to look things over together and rework to something non-disgusting :)

@kevaundray kevaundray Dec 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh god :)

@kevaundray kevaundray Dec 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bear in mind that if a release is made, then this code will make its way into Noir user's hands -- haven't looked over the changes, though it seems that the solidity verifier contract may no longer work for them if this were to be merged?

@codygunton codygunton Dec 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok don't worry, we just want to get it working in this branch, won't merge

@codygunton codygunton force-pushed the cg-lde/expose-goblin branch 2 times, most recently from 7999299 to cd99d53 Compare December 12, 2023 18:21
ludamad
ludamad reviewed Dec 12, 2023
View reviewed changes
Comment thread barretenberg/acir_tests/flows/prove_and_verify.sh Outdated
# It may not catch all class of bugs.
$BIN prove_and_verify $VFLAG -c $CRS_PATH -b ./target/acir.gz No newline at end of file
$BIN prove_and_verify $VFLAG -c $CRS_PATH -b ./target/acir.gz
# lldb-16 -- $BIN prove_and_verify $VFLAG -c $CRS_PATH -b ./target/acir.gz

@ludamad ludamad Dec 12, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed

@AztecBot

AztecBot commented Dec 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Benchmark results

No metrics with a significant change found.

Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit a60b71a5 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 8 txs 32 txs 128 txs
l1_rollup_calldata_size_in_bytes 45,444 179,588 716,132
l1_rollup_calldata_gas 222,984 867,776 3,449,420
l1_rollup_execution_gas 842,071 3,594,884 22,205,082
l2_block_processing_time_in_ms 1,357 (+1%) 5,338 (+4%) 21,175
note_successful_decrypting_time_in_ms 344 1,093 (-2%) 3,916 (+2%)
note_trial_decrypting_time_in_ms 55.4 (+4%) 37.3 (-1%) 138
l2_block_building_time_in_ms 13,731 (+1%) 53,698 (-1%) 216,870
l2_block_rollup_simulation_time_in_ms 10,182 (+2%) 39,716 (-1%) 160,510
l2_block_public_tx_process_time_in_ms 3,515 13,914 56,113

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric 5 blocks 10 blocks
node_history_sync_time_in_ms 15,706 (-2%) 29,393
note_history_successful_decrypting_time_in_ms 2,553 (+1%) 5,170 (+2%)
note_history_trial_decrypting_time_in_ms 99.3 (+17%) 151 (+8%)
node_database_size_in_bytes 3,910,865 4,256,099
pxe_database_size_in_bytes 29,940 59,499

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 199 43,109 20,441
private-kernel-ordering 114 (-1%) 25,833 9,689
base-rollup 2,055 244,144 881
root-rollup 84.8 (-4%) 4,088 889
private-kernel-inner 260 64,516 20,441
public-kernel-private-input 171 (-1%) 25,203 20,441
public-kernel-non-first-iteration 170 (-1%) 25,245 20,441
merge-rollup 11.1 (-2%) 2,608 881

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric 0 deployed contracts 1 deployed contracts
tx_size_in_bytes 10,323 26,359

@ludamad ludamad force-pushed the cg-lde/expose-goblin branch from 3d3a4f9 to f71424c Compare December 13, 2023 19:59
codygunton and others added 23 commits December 17, 2023 20:42
ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/dsl/acir_proofs/acir_composer.hpp Outdated
ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/dsl/acir_proofs/major-todos.md Outdated
Major TODOS
acir_format::Composer (and other types) become GUH
? stretch update create_circuit to use Goblin gates (build_contraints and circuit_buf_to_acir_format in particular?)
Update AcirComposer

@ludamad ludamad Dec 18, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be deleted?

@ledwards2225 ledwards2225 Dec 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted

ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/goblin/goblin.cpp Outdated
ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/goblin/goblin.hpp Outdated
ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/proof_system/circuit_builder/ultra_circuit_builder.hpp Outdated
ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/proof_system/debug_utility.hpp
@@ -0,0 +1,43 @@
#pragma once

#include "barretenberg/common/log.hpp"

@ludamad ludamad Dec 18, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: usually there's a better name than X_utility. But to be clear, it's holidays, get'er in

@ledwards2225 ledwards2225 Dec 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to instance_inspector - useful for now but may delete in the near future

ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/ultra_honk/ultra_composer.hpp Outdated
ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/cpp/src/barretenberg/ultra_honk/ultra_verifier.cpp
ludamad
ludamad reviewed Dec 18, 2023
View reviewed changes
Comment thread barretenberg/ts/src/crs/node/ignition_files_crs.ts
* The path to our SRS object, assuming that we are in e.g. barretenberg/ts/dest/node/crs/node folder.
*/
export const SRS_DEV_PATH = getCurrentDir() + '/../../../cpp/srs_db/ignition/monomial';
export const SRS_DEV_PATH = getCurrentDir() + '/../../../../../cpp/srs_db/ignition/monomial';

@ludamad ludamad Dec 18, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old path actually never found the file, always downloaded

@ludamad ludamad marked this pull request as ready for review December 18, 2023 22:22
@ledwards2225 ledwards2225 merged commit d093266 into master Dec 18, 2023
@ledwards2225 ledwards2225 deleted the cg-lde/expose-goblin branch December 18, 2023 23:18
@AztecBot AztecBot mentioned this pull request Dec 18, 2023
Merged
@kevaundray

kevaundray commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

Wait why has this now been merged?

see: #3636 (comment)

@ledwards2225

ledwards2225 commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

Wait why has this now been merged?

see: #3636 (comment)

Not to worry! This still isn't hooked up to anything real, it just runs a few new tests using Goblin. Sorry - poor communication on our part. When Cody said "this wont be merged" we were still thinking it wouldn't be. Since then we decided to do something more isolated very much for the purpose of getting it merged

@ledwards2225 ledwards2225 mentioned this pull request Jan 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ledwards2225 ledwards2225 ledwards2225 left review comments

@ludamad ludamad ludamad approved these changes

+1 more reviewer

@kevaundray kevaundray kevaundray left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ludamad ludamad

@codygunton codygunton

@ledwards2225 ledwards2225

Labels

None yet

Projects

No open projects
A3
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@codygunton @AztecBot @kevaundray @ledwards2225 @ludamad @ludamad0